feat: Add bmake pre-commit hook #3998
Conversation
Signed-off-by: Muhammad Hassaan Saleem <iamhassaans@gmail.com>
Summary by CodeRabbit
WalkthroughThis PR introduces Makefile linting via mbake by adding a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Muhammad Hassaan Saleem <iamhassaans@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.pre-commit-config.yaml (2)
55-55:language_version: systemties markdownlint to the ambient Node.js installation.This is a behavioural change: pre-commit will now use whatever
nodeis on the systemPATHrather than a version it manages itself. This is fine onubuntu-latest(Node is pre-installed), but locally a developer without Node in theirPATHwill see the hook fail with a confusing error. If a pinned Node version is desired, uselanguage_version: '24'(or another explicit version) to stay consistent with thecheck-frontendjob; if the intent is simply to avoid a second managed runtime, document that assumption.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.pre-commit-config.yaml at line 55, Replace the ambiguous hook setting "language_version: system" in the markdownlint pre-commit hook with an explicit Node version string (for example "language_version: '24'") to match the pinned runtime used by the check-frontend job, or alternatively add a comment/documentation near the markdownlint hook stating that the hook deliberately relies on the system PATH node installation; update the entry that currently contains language_version: system to the chosen explicit version or add the documentation so developers without Node in PATH don’t encounter confusing failures.
2-5: Consider updating the checkmake repo URL to the canonical location.The hook points to
https://github.com/mrtazz/checkmake, which is the pre-transfer URL. The canonical checkmake repository now lives athttps://github.com/checkmake/checkmake— GitHub currently redirects the old URL, but using the canonical URL is more explicit and avoids a redirect dependency.♻️ Suggested update
- - repo: https://github.com/mrtazz/checkmake + - repo: https://github.com/checkmake/checkmake rev: v0.3.2 hooks: - id: checkmake🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.pre-commit-config.yaml around lines 2 - 5, Replace the non-canonical checkmake repo URL in .pre-commit-config.yaml: update the repo string "https://github.com/mrtazz/checkmake" to the canonical "https://github.com/checkmake/checkmake" in the pre-commit hook block (the entry containing rev: v0.3.2 and id: checkmake) so the hook points directly to the official repository and avoids GitHub redirects..github/workflows/run-ci-cd.yaml (1)
47-51: Pingo-versionto a specific release for reproducibility; cache key doesn't account for version changes.Using
'stable'means any run after a new stable Go release will pick up a different version, creating non-determinism in how checkmake is compiled within the pre-commit sandbox. The pre-commit cache key (line 56) only includes.pre-commit-config.yamlandrunner.os, so if the stable Go version changes, a stale cached binary could be served until manual invalidation or the config file changes. Consider pinning to a specific version (e.g.,'1.24') instead.The SHA
0c52d547c9bc32b1aa3301fd7a9cb496313a4491correctly corresponds toactions/setup-goreleasev5.0.0and is properly pinned; no update needed there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/run-ci-cd.yaml around lines 47 - 51, The workflow currently sets go-version: 'stable' in the actions/setup-go step (uses: actions/setup-go@0c52d547c9bc32b1aa3301fd7a9cb496313a4491), which causes nondeterministic Go versions; change go-version to a specific release string (e.g., '1.24') and also update the pre-commit cache key construction (the cache key that currently references .pre-commit-config.yaml and runner.os) to include the pinned go-version so cache invalidation aligns with Go upgrades, ensuring reproducible builds of checkmake in the pre-commit sandbox.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/run-ci-cd.yaml:
- Around line 47-51: The workflow currently sets go-version: 'stable' in the
actions/setup-go step (uses:
actions/setup-go@0c52d547c9bc32b1aa3301fd7a9cb496313a4491), which causes
nondeterministic Go versions; change go-version to a specific release string
(e.g., '1.24') and also update the pre-commit cache key construction (the cache
key that currently references .pre-commit-config.yaml and runner.os) to include
the pinned go-version so cache invalidation aligns with Go upgrades, ensuring
reproducible builds of checkmake in the pre-commit sandbox.
In @.pre-commit-config.yaml:
- Line 55: Replace the ambiguous hook setting "language_version: system" in the
markdownlint pre-commit hook with an explicit Node version string (for example
"language_version: '24'") to match the pinned runtime used by the check-frontend
job, or alternatively add a comment/documentation near the markdownlint hook
stating that the hook deliberately relies on the system PATH node installation;
update the entry that currently contains language_version: system to the chosen
explicit version or add the documentation so developers without Node in PATH
don’t encounter confusing failures.
- Around line 2-5: Replace the non-canonical checkmake repo URL in
.pre-commit-config.yaml: update the repo string
"https://github.com/mrtazz/checkmake" to the canonical
"https://github.com/checkmake/checkmake" in the pre-commit hook block (the entry
containing rev: v0.3.2 and id: checkmake) so the hook points directly to the
official repository and avoids GitHub redirects.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.pre-commit-config.yaml (1)
2-5: Consider updating to the canonical repository URL.
v0.3.2is confirmed to be the latest release, so the pinned revision is current and valid. The hook configuration is also correct: checkmake defaults--configtocheckmake.iniin the CWD and picks it up automatically, so no explicitargsare needed here.The one thing to tidy up:
mrtazz/checkmakewas transferred tocheckmake/checkmake. GitHub redirects are transparent for most operations, but referencing the canonical URL is more robust.♻️ Update to canonical repo URL
- - repo: https://github.com/mrtazz/checkmake + - repo: https://github.com/checkmake/checkmake rev: v0.3.2 hooks: - id: checkmake🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.pre-commit-config.yaml around lines 2 - 5, Update the pre-commit hook to use the canonical repository URL: replace the repo string "https://github.com/mrtazz/checkmake" with "https://github.com/checkmake/checkmake" while keeping the same pinned revision "rev: v0.3.2" and the existing hook id "checkmake" (no changes to args/config needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.pre-commit-config.yaml:
- Around line 2-5: Update the pre-commit hook to use the canonical repository
URL: replace the repo string "https://github.com/mrtazz/checkmake" with
"https://github.com/checkmake/checkmake" while keeping the same pinned revision
"rev: v0.3.2" and the existing hook id "checkmake" (no changes to args/config
needed).
|
@cubic-dev-ai |
@hassaansaleem28 I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Any reasons to pick this over https://github.com/EbodShojaei/bake ?
I'd prefer less dependencies (this PR introduces golang which is okay but it's better to avoid extra lang support if possible)
Signed-off-by: Muhammad Hassaan Saleem <iamhassaans@gmail.com>
hassaansaleem28
dismissed stale reviews from cubic-dev-ai[bot] and coderabbitai[bot]
via
b563223
Signed-off-by: Muhammad Hassaan Saleem <iamhassaans@gmail.com>
hassaansaleem28
force-pushed
the
feat/add-makefile-linter-checkmake
branch
from
9bdf224 to
29d03fe
Compare
|
|
Hi @arkid15r, I disabled end-of-file-fixer for Makefiles. mbake-format strips the final newline, and the fixer adds it back, creating an infinite loop. I let mbake win. |
hassaansaleem28
changed the title
|
@cubic-dev-ai |
@hassaansaleem28 I have started the AI code review. It will take a few minutes to complete. |
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.bake.toml:
- Around line 1-25: The PR has a tool identity mismatch: the implementation adds
an mbake (.bake.toml) configuration but the title/description/issue reference
checkmake (Go) and a checkmake.ini; fix by reconciling intent — either (A)
implement checkmake integration: add the checkmake config file (checkmake.ini),
update CI to use setup-go and install/checkcheckmake, and replace/rename
.bake.toml references with checkmake-specific config and invocation, or (B)
change the PR metadata to accurately describe using mbake: update the PR
title/description/linked issue text to mention mbake (EbodShojaei/bake), remove
any references to checkmake/checkmake.ini/setup-go, and ensure CI/workflows and
documentation reflect .bake.toml and mbake commands; update any filenames and
README entries so code (symbols: .bake.toml, checkmake.ini, setup-go, mbake) and
PR text are consistent.
In @.pre-commit-config.yaml:
- Around line 2-3: The pre-commit entry uses a mutable tag for the third-party
repo (repo: https://github.com/EbodShojaei/bake, rev: v1.4.5); replace the tag
with the exact commit SHA for v1.4.5 (obtain via git ls-remote
https://github.com/EbodShojaei/bake refs/tags/v1.4.5) and update the rev value
to that SHA in .pre-commit-config.yaml, then add a short comment next to the
repo entry noting the tag and the resolved SHA for future traceability.
| - repo: https://github.com/EbodShojaei/bake | ||
| rev: v1.4.5 |
There was a problem hiding this comment.
Supply chain risk: personal repository pinned by mutable tag, not by commit SHA.
https://github.com/EbodShojaei/bake is a personal GitHub repository (not an established org/foundation project), and rev: v1.4.5 is a git tag that can be silently re-pointed to a different commit. Pre-commit hooks execute code on every developer's machine on every commit, making this a meaningful supply chain exposure.
Recommend pinning to the specific commit SHA that corresponds to v1.4.5 and adding a comment for traceability:
🔒 Proposed fix: pin to commit SHA
- repo: https://github.com/EbodShojaei/bake
- rev: v1.4.5
+ rev: <full-40-char-SHA-of-v1.4.5-tag> # v1.4.5Retrieve the SHA with:
git ls-remote https://github.com/EbodShojaei/bake refs/tags/v1.4.5🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.pre-commit-config.yaml around lines 2 - 3, The pre-commit entry uses a
mutable tag for the third-party repo (repo: https://github.com/EbodShojaei/bake,
rev: v1.4.5); replace the tag with the exact commit SHA for v1.4.5 (obtain via
git ls-remote https://github.com/EbodShojaei/bake refs/tags/v1.4.5) and update
the rev value to that SHA in .pre-commit-config.yaml, then add a short comment
next to the repo entry noting the tag and the resolved SHA for future
traceability.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3998 +/- ##
=======================================
Coverage 99.79% 99.79%
=======================================
Files 517 517
Lines 15914 15914
Branches 2168 2168
=======================================
Hits 15881 15881
Misses 3 3
Partials 30 30
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
LGTM
Thanks for adding this!
2 participants
Proposed change
Resolves #3984
Description
This PR integrates checkmake (a Makefile linter) into the pre-commit configuration and CI pipeline to enforce consistency and correctness across all Makefiles in the repository.
Changes
minphony,maxbodylength,uniquetargets).actions/setup-go(pinned to SHA) in the pre-commit job, enabling checkmake to run in CI..PHONYdeclarations to all Makefiles to satisfy linter requirements and prevent file conflicts.Verification
pre-commit run checkmake --all-fileslocally: Passed.make checklocally: Passed.Checklist
make check-testlocally: all warnings addressed, tests passed